Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(): searchPossibleTargets targets value #9343

Closed
wants to merge 11 commits into from
Closed

fix(): searchPossibleTargets targets value #9343

wants to merge 11 commits into from
+911 −81

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Sep 15, 2023 •
edited
Loading

Motivation

Trying to impl a fix for the problem presented in #9329
Being that the group rewrite made searchPossibleTargets return a sub target instead of the found taregt to allow deep selection. This surfaced an issue that the use of Canvas#targets aka subTargets is wrong and confusing.

closes #9329

Description

Rethinking and Reconsidering subTargets

If I go by the name and from what I understand was the original intention I believe subTargets should populate the sub targets of the found target.
Parents of the found target should not be part of subTargets, that is what makes the name confusing. They should not be part mainly because it is clear that they are in the hit region or else the found target would not be found. If anyone including fabric needs the parents it is very simple to walk up the tree.

The main use in fabric of subTargets is for events. So we need to fix that.
I think it is unclear what is expected from the synthetic events.

Changes

  • _searchPossibleTargets => findTargetsTraversal + added a flag to allow searching for all hits
  • fix searchPossibleTargets return value to consider an case where subTargetCheck: true, interactive: false group were travesred, so returning the first hit is wrong
  • dep searchPossibleTargets => findTargets
  • move setting targets to findTarget because it varies on the case and is a step towards refactoring the use of Canvas#targets

Gist

The tests came from #9333 if you wish to merge that first, conflicts should resolve to ours

In Action

@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2023 •
edited
Loading

Build Stats

file / KB (diff) bundled minified
fabric 916.123 (+0.468) 305.800 (+0.330)

@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2023 •
edited
Loading

Coverage after merging subtargets2 into master will be

82.97%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 35, 38, 41
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.71%94.64%86.67%97.09%101, 104, 207–208, 233–234
   CommonMethods.ts96.55%87.50%100%100%9
   Intersection.ts100%100%100%100%
   Observable.ts87.76%85.29%87.50%89.58%134–135, 160–161, 32–33, 41, 50, 78, 89
   Point.ts100%100%100%100%
   Shadow.ts98.36%95.65%100%100%178
   cache.ts97.06%90%100%100%57
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
src/Pattern
   Pattern.ts92.31%91.89%90%93.55%118, 129, 138, 31, 94
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 13, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.06%82.35%100%93.81%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 149, 149, 149–152, 154–157, 161–162, 164, 166–169, 17, 172, 179, 18, 180, 182, 184–185, 187, 194–195, 197–198, 20, 201, 201, 208, 208, 21, 212, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts79.14%77.81%83.05%79.57%1002–1003, 1003, 1003, 1005, 1013, 1013, 1013–1015, 1015, 1015, 1022–1023, 1031–1032, 1032, 1032–1033, 1039, 1041, 1072–1074, 1077–1078, 1082–1083, 1196–1198, 1201–1202, 1275, 1394, 1516, 157, 182, 292–293, 296–300, 305, 328–329, 334–339, 359, 359, 359–360, 360, 360–361, 369, 37, 374–375, 375, 375–376, 378, 387, 393–394, 394, 394, 41, 436, 444, 448, 448, 448–449, 451, 534–535, 535, 535–536, 542, 542, 542–544, 564, 566, 566, 566–567, 567, 567, 570, 570, 570–571, 574, 583–584, 586–587, 589, 589–590, 592–593, 605–606, 606, 606–607, 609–614, 620, 627, 664, 664, 664, 666, 668–673, 679, 685, 685, 685–686, 688, 691, 696, 709, 737, 737, 795–796, 796, 796–797, 799, 802–803, 803, 803–804, 806–807, 810, 810–812, 815–816, 886, 898, 905, 926, 958, 979–980, 996–997, 997, 997–999
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts93.62%90.87%92.59%95.95%1005, 1013, 1132, 1134, 1136–1137, 301, 478–479, 481–482, 482, 482, 531–532, 593–594, 647–649, 681, 686–687, 714–715, 764, 842, 865, 865, 887, 932, 932–933, 936, 956, 956
   StaticCanvas.ts96.78%93.09%100%98.53%1030, 1040, 1092–1093, 1096, 1131–1132, 1208, 1217, 1217, 1221, 1221, 1268–1269, 185–186, 202, 570, 582–583, 913–914, 914, 914–915
   StaticCanvasOptions.ts100%100%100%100%
   TextEditingManager.ts84.31%71.43%91.67%88%17–18, 18, 18–19, 19, 19
src/canvas/DOMManagers
   CanvasDOMManager.ts95.52%70%100%100%21–22, 29
   StaticCanvasDOMManager.ts100%100%100%100%
   util.ts86.67%80.56%83.33%93.94%14, 26, 63–64, 67, 67, 74, 93–94
src/color
   Color.ts94.96%91.67%96.30%96.05%233, 258–259, 267–268, 48
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts85.71%76.92%100%89.74%55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
   Control.ts93.33%87.88%91.67%97.78%175, 240, 327, 327, 362
   changeWidth.ts100%100%100%100%
   commonControls.ts100%100%100%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   drag.ts100%100%100%100%
   fireEvent.ts88.89%75%100%100%13
   polyControl.ts6.06%0%0%11.43%103, 117, 117, 117, 117, 117, 119–122, 122, 125, 132, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–84, 84, 86, 89–90, 90, 90, 90, 90, 92, 97
   rotate.ts19.57%12.50%50%21.43%41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
   scale.ts93.57%92.94%100%93.67%129–130, 132–134, 148–149, 181–183, 42
   scaleSkew.ts78.79%64.29%100%85.71%27, 29, 29, 29, 31, 33, 35
   skew.ts91.03%79.31%100%97.67%131–132, 163–164, 171, 177, 179
   util.ts100%100%100%100%
   

ShaMan123
ShaMan123 commented Sep 15, 2023
View reviewed changes
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good incremental fix but there are a few more things to address here.
Even though all tests pass it does not mean that event firing is correct.
Seems there are no tests covering these cases.

src/canvas/Canvas.ts Outdated Show resolved Hide resolved
src/canvas/SelectableCanvas.ts Outdated Show resolved Hide resolved
src/canvas/SelectableCanvas.ts Outdated Show resolved Hide resolved
src/canvas/__tests__/eventData.test.ts
Copy link
Contributor Author

@ShaMan123 ShaMan123 Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migrated all findTarget tests, see 0c04292
I have decided not to remove the old ones yet so we prove the PR passes (eventhough I did a thorough job of migrating the tests). I kept the names of the tests the same to make it easy to trace back.
c1448b0 contains new tests about my concerns raised in the review FIXED by 40c725b

src/canvas/__tests__/eventData.test.ts Outdated Show resolved Hide resolved
@ShaMan123 ShaMan123 requested a review from asturur September 15, 2023 06:12
@ShaMan123 ShaMan123 mentioned this pull request Sep 15, 2023
Merged
@ShaMan123 ShaMan123 force-pushed the subtargets2 branch 2 times, most recently from 582b545 to 6e9942d Compare September 15, 2023 07:55
ShaMan123
ShaMan123 commented Sep 15, 2023
View reviewed changes
Copy link
Contributor Author

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • dep searchPossibleTargets => findTargets
  • move setting targets to findTarget because it varies on the case and is a step towards refactoring the use of Canvas#targets

Now I am happy with this PR

@asturur asturur removed the group label Jan 14, 2024
@ShaMan123
Copy link
Contributor Author

Ported to ShaMan123#5
Contributions are welcome

@ShaMan123 ShaMan123 closed this May 7, 2024
@ShaMan123 ShaMan123 deleted the subtargets2 branch May 7, 2024 18:24
@asturur asturur restored the subtargets2 branch May 7, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asturur asturur Awaiting requested review from asturur

Assignees
No one assigned
Labels
selection
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ShaMan123 @asturur